-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Logs UI] Add shared observability page template and navigation #99380
Conversation
c75c61d
to
73de772
Compare
/oblt-deploy |
@elasticmachine merge upstream |
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
@katefarrar we'd appreciate a review from the design perspective as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes all look good to me 🎉
bodyColor={theme.eui.euiPageBackgroundColor} | ||
> | ||
<CentralizedFlexGroup> | ||
<ObservabilityPageTemplate template="centeredBody"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment, because this works just fine, but the <KibanaPageTemplate />
accepts a isEmptyState
prop, if set to true it will look at which content exists (children, header etc) and determine the correct template itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I wonder, though, if semantically the loading screen qualifies as an empty state.
@cchaos @katefarrar can you provide any guidance on how to best render full-page loading states with the <KibanaPageTemplate />
? We're currently setting template="centeredBody"
to display a centered spinner with accompanying loading text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't anything that the Eui/KibanaPageTemplate provides specifically semantically to indicate empty state. It's purely a visual representation for consistency. Do you have a screenshot of your loading state that I could see for better advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I noticed the same when I was testing it out. The original loading state sat within a small panel, but now with the new page component, I think it'd make more sense to remove the panel around the loading message and show the content area background? With this new layout, we're always expecting to have the main content area there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weltenwort I wonder where the background is coming from around the spinner and text? If it's a panel, it supports color="transparent"
to avoid it adding that light grey background.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems I can remove it using pageContentProps={{ color: 'transparent' }}
on the template. Does it make sense to deviate from the EUI defaults, though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably shouldn't go down that route. Let's leave as is, but I'll speak to @cchaos on how we want to handle full page loading states in the new layouts. Thanks for taking a look, and for fixing the original style issue 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked it over with @formgeist and I'll try to build something into the page template. Though it will most likely still follow the same pattern as the empty state, so I'd suggest using that for now until we have something specific for loading.
/oblt-deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weltenwort Great job on getting the new navigation in - can't wait to get it into all of the apps, so you have the full experience. I noticed some minor things we probably should look into;
- If an app is hidden in the Spaces settings, the navigation isn't hidden. The Kibana navigation item is, but the section and links still show, so we should make sure we respect the Space settings.
- I mentioned the initial loading state style in a comment above [Logs UI] Add shared observability page template and navigation #99380 (comment) but I guess that comes with us moving from the full page layout to this new split pane design where the background is the same as our panels. We can certainly fix this in another PR related to the Overview, just pointing it out.
Otherwise, I think this is good to go 👍
Thanks for the review, @formgeist!
The patch to add these menu entries is just for demonstration purposes and not part of the PR. Since the side nav can't know about the relation between specific permissions and menu entries, it's up to the registering plugin to guard their registrations. The README puts it like this:
Oh, that panel padding in the loading indicator doesn't look right. I'll take a look at removing the wrapping panel. |
Ah, good to know!
Thanks! |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsasync chunk count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: cc @weltenwort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still having issues with my local environment, but I was able to look at this with @formgeist. Everything looks great. Thank you all! 👍
…tic#99380) Co-authored-by: Kerry Gallagher <471693+Kerry350@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…) (#100792) Co-authored-by: Kerry Gallagher <471693+Kerry350@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com> Co-authored-by: Kerry Gallagher <471693+Kerry350@users.noreply.github.com>
📝 Summary
This exposes a navigation registry and a shared page template on the observability
setup()
andstart()
contract respectively.closes #98212
🎨 Previews
🕵️ Review notes
To cause the logs and metrics apps to show up in the sidebar, the following patch could be applied:
Progress